fix: collection hash indexing #104
Conversation
| // TODO: Support collection unverification. | ||
| // We will likely need to nullify the collection field so we can maintain | ||
| // the seq value and avoid out-of-order indexing bugs. |
There was a problem hiding this comment.
I originally was thinking along this route but ended up just adding a verified field the asset_grouping table, and then changing the Read API calls to filter on Verified.eq(true). I did that because it is more consistent with the asset_creators table approach and the contract does in fact have the concept of verify so it made sense to me.
Maybe after your change to blockbuster, I would not need a separate sequence number because i could update collection and slot_updated when just doing VerifyCollection and UnverifyCollection, all protected by a single sequence number. Either way I need to make some change based on the blockbuster change to pick up the new data hash. We should chat offline about approach before I merge my PR.
Here is my PR for reference: metaplex-foundation#90
There was a problem hiding this comment.
The only thing to factor in is that creators can be removed, so we need to be able to support that case too.
There was a problem hiding this comment.
yeah I agree nullification is better future proofing for when we have metadata updates
| &[v], | ||
| id_bytes.clone().as_ref(), | ||
| e.owner.as_ref().unwrap(), | ||
| e.delegate.unwrap_or(Default::default()).as_ref(), |
There was a problem hiding this comment.
The problem could be because for the txs being used in testing, owner == delegate. And then in our off-chain data store we do NOT save a delegate (we set it to NULL and you would end up getting Default::default() here). But on chain would require the actual delegate pubkey to be used.
So I think you might need to do something like e.delegate.unwrap_or(e.owner.as_ref().unwrap())
There was a problem hiding this comment.
Ah ok thank you, let me try that!
| id_bytes.clone().as_ref(), | ||
| e.owner.as_ref().unwrap(), | ||
| e.delegate.unwrap_or(Default::default()).as_ref(), | ||
| cl.index.to_le_bytes().as_ref(), |
There was a problem hiding this comment.
Another thing that could be the issue is if this index might not be correct. Isn't this supposed to be the nonce value rather than the cl index value? So in this case would it be like e.nonce?
There was a problem hiding this comment.
There is no nonce in the asset data. I believe nonce == leaf id
There was a problem hiding this comment.
Yeah nonce in the actual asset table gets called leaf_id when returned from the API
| .to_owned(), | ||
| ) | ||
| .build(DbBackend::Postgres); | ||
| query.sql = format!( | ||
| "{} WHERE excluded.slot_updated > asset_grouping.slot_updated AND excluded.seq >= asset_grouping.seq", |
There was a problem hiding this comment.
Yeah not sure if we want the slot_updated here because there could be multiple tx in the same slot when just dealing with bubblegum transactions?
* Updating test workflow * Adding some annotations * Fixing path issue. * Fixing fetch_trees name * Adding push condition * Main (#104) * Fix raw name build (#122) * Added fetch trees tool (#123) Fetches a list of trees from an RPC endpoint. --------- Co-authored-by: Linus Kendall <[email protected]> --------- Co-authored-by: Linus Kendall <[email protected]>
Overview
5ZKjPxm3WAZzuqqkCDjgKpm9b5XjB9cuvv68JvXxWThvJaJxcMJgpSbYs4gDA9dGJyeLzsgNtnS6oubANF1KbBm" for asset2WjoMU1hBGXv8sKcxQDGnu1tgMduzdZEmEEGjh8MZYfC. This txn sets and verifies a collection for a cNFT that does not have a collection.7nK9a2DSDZ4Gh6DatmxGJmuLiDEswaY9bYSSPTtQppk7PtLKXYE84jWzm7AC4G1fpa831GaXuXcn5n5ybWqB4e5and asset2gEbvG3Cb6JRaGWAx5e85Bf5z4u37EURBeyPBqXDzZoY. This txn unverifies a cNFT with a verified collection.Testing